Skip to content

Conversation

@david-roper
Copy link
Collaborator

@david-roper david-roper commented Dec 24, 2025

works on issue #1103

allows for the statistics cards to be interacted with

shows number of users, available instrumentes or number of records for each instrument.

Summary by CodeRabbit

  • New Features
    • Dashboard metrics (Users, Instruments, Records) are interactive — click tiles to open detailed modal dialogs with headers, lists, and loading/error states.
    • Smooth animated modal transitions and animated list entries when viewing metric details.
    • Total Subjects tile supports click-to-navigate to a detailed view.
    • Records/Sessions trend now uses combined computed data for more accurate charts.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 24, 2025

Walkthrough

Replaces three static statistic tiles with Dialog-driven modals (Users, Instruments, Records); wires three new data hooks, local modal state, animated modal lists, loading/error handling, and a Records/Sessions trend view; adds click-to-navigate on the Subjects tile.

Changes

Cohort / File(s) Summary
Dashboard component
apps/web/src/routes/_app/dashboard.tsx
Wraps StatisticCard tiles with Dialog triggers and local boolean state; adds useNavigate for Subjects navigation; integrates AnimatePresence/motion for animated modal lists; shows Spinner for loading and error handling; composes trend chart data.
New data hooks
@/hooks/useInstrumentInfoQuery, @/hooks/useInstrumentRecords, @/hooks/useUsersQuery
Adds and exports three data-fetching hooks used by the dashboard; useInstrumentRecords is called with enabled/group-scoped params and used to compute instrumentData, instrumentInfo, recordIds, and recordCounter.
Imports & UI plumbing
apps/web/src/routes/_app/dashboard.tsx (imports)
Adds imports: Dialog, Spinner, AnimatePresence, motion, and the three new hooks; no external API/type signature changes.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant Dashboard
  participant DataHooks as DataHooks
  participant DialogUI as DialogUI
  participant Navigator

  User->>Dashboard: Clicks "Users"/"Instruments"/"Records" tile
  Dashboard->>DialogUI: open dialog (show Spinner while loading)
  Dashboard->>DataHooks: call hook (useUsersQuery / useInstrumentInfoQuery / useInstrumentRecords)
  DataHooks-->>Dashboard: return data or loading/error
  Dashboard->>DialogUI: render animated list (AnimatePresence/motion) and trend chart
  DialogUI-->>User: display modal content
  User->>DialogUI: selects item
  DialogUI->>Dashboard: emit item click event
  Dashboard->>Navigator: navigate to route (e.g., /datahub)
  Navigator-->>User: route change
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • #1180: Modifies the same dashboard component (charts/trends) and may intersect with these modal and data-fetching changes.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Responsive dashboard' is vague and doesn't clearly convey the main changes: adding modal-based interactions to dashboard tiles with data hooks for users, instruments, and records. Use a more specific title like 'Add interactive modals to dashboard statistics tiles' or 'Make dashboard tiles interactive with modal dialogs and data queries'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@david-roper david-roper marked this pull request as ready for review December 31, 2025 16:12
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
apps/web/src/routes/_app/dashboard.tsx (5)

33-38: Redundant enabled: true.

Queries are enabled by default in TanStack Query. This can be removed unless you need it for future conditional fetching.

🔎 Suggested fix
  const recordsQuery = useInstrumentRecords({
-   enabled: true,
    params: {
      groupId: currentGroup?.id
    }
  });

54-62: Consider memoizing derived data to avoid recomputation on every render.

recordCounter iterates over recordIds for each instrument (O(n×m)). For larger datasets, this could cause unnecessary re-renders.

🔎 Suggested optimization
+import { useMemo } from 'react';
+
+// Inside component:
+const recordCounter = useMemo(() => {
+  if (!recordsQuery.data || !instrumentInfo) return [];
+  const counts = new Map<string, number>();
+  for (const record of recordsQuery.data) {
+    counts.set(record.instrumentId, (counts.get(record.instrumentId) ?? 0) + 1);
+  }
+  return instrumentInfo.map((inst) => ({
+    count: counts.get(inst.id) ?? 0,
+    instrumentTitle: inst.title
+  }));
+}, [recordsQuery.data, instrumentInfo]);

278-305: Static header inside AnimatePresence and potentially non-unique key.

The header row (lines 279-288) shouldn't be inside AnimatePresence since it's not animated. Also, instrument.title may not be unique—prefer instrument.id as the key.

🔎 Suggested fix
  <ul className="flex flex-col gap-5 overflow-auto">
+   <div className="flex justify-between gap-4 font-bold">
+     <p>{t({ en: 'Title', fr: 'Titre' })}</p>
+     <p>{t({ en: 'Kind', fr: 'Genre' })}</p>
+   </div>
+   <hr />
    <AnimatePresence mode="popLayout">
-     <div className="flex justify-between gap-4 font-bold">
-       ...
-     </div>
-     <hr></hr>
      {instrumentInfo?.map((instrument, i) => {
        return (
          <motion.li
            ...
-           key={instrument.title}
+           key={instrument.id}

336-365: Same structural issues as instruments modal.

Static header is inside AnimatePresence, and instrument.instrumentTitle may not be unique. Apply similar fixes: move header outside AnimatePresence and use a unique key (e.g., combine title with index if no ID available). Also consider adding loading state for recordsQuery.


214-214: Staggered animation delay may feel sluggish with many items.

delay: 0.15 * i accumulates—20 items means the last one starts animating after 3 seconds. Consider capping the delay or reducing the multiplier for snappier UX.

🔎 Example cap
- transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
+ transition={{ bounce: 0.2, delay: Math.min(0.1 * i, 0.5), duration: 0.8, type: 'spring' }}
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0305a0 and c77f9be.

📒 Files selected for processing (1)
  • apps/web/src/routes/_app/dashboard.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: lint-and-test
🔇 Additional comments (1)
apps/web/src/routes/_app/dashboard.tsx (1)

1-31: LGTM!

Imports and state declarations are well-organized.

Comment on lines 204 to 223
<ul className="flex flex-col gap-5 overflow-auto">
<AnimatePresence mode="popLayout">
{userInfoQuery.data?.map((user, i) => {
return (
<motion.li
layout
animate={{ opacity: 1, y: 0 }}
exit={{ opacity: 0 }}
initial={{ opacity: 0 }}
key={user.username}
transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
>
<div className="flex justify-between gap-4">
<p>{user.username}</p>
</div>
</motion.li>
);
})}
</AnimatePresence>
</ul>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing loading/error states in modal content.

If userInfoQuery is still fetching or fails, the modal displays empty content with no feedback. Consider adding loading spinners or error messages.

🔎 Example addition
  <ul className="flex flex-col gap-5 overflow-auto">
+   {userInfoQuery.isLoading && <p>Loading...</p>}
+   {userInfoQuery.isError && <p>Failed to load users</p>}
    <AnimatePresence mode="popLayout">
      {userInfoQuery.data?.map((user, i) => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<ul className="flex flex-col gap-5 overflow-auto">
<AnimatePresence mode="popLayout">
{userInfoQuery.data?.map((user, i) => {
return (
<motion.li
layout
animate={{ opacity: 1, y: 0 }}
exit={{ opacity: 0 }}
initial={{ opacity: 0 }}
key={user.username}
transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
>
<div className="flex justify-between gap-4">
<p>{user.username}</p>
</div>
</motion.li>
);
})}
</AnimatePresence>
</ul>
<ul className="flex flex-col gap-5 overflow-auto">
{userInfoQuery.isLoading && <p>Loading...</p>}
{userInfoQuery.isError && <p>Failed to load users</p>}
<AnimatePresence mode="popLayout">
{userInfoQuery.data?.map((user, i) => {
return (
<motion.li
layout
animate={{ opacity: 1, y: 0 }}
exit={{ opacity: 0 }}
initial={{ opacity: 0 }}
key={user.username}
transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
>
<div className="flex justify-between gap-4">
<p>{user.username}</p>
</div>
</motion.li>
);
})}
</AnimatePresence>
</ul>
🤖 Prompt for AI Agents
apps/web/src/routes/_app/dashboard.tsx around lines 204-223: the modal list
renders nothing when userInfoQuery is loading or errored; update the JSX to
conditionally render a loading state (use userInfoQuery.isLoading to show a
spinner/placeholder), an error state (use userInfoQuery.isError and show
userInfoQuery.error.message or a friendly message), and an explicit empty-state
when data is an empty array; only render the AnimatePresence/motion list when
userInfoQuery.data?.length > 0, and ensure the loading/error/empty states are
accessible (visible text and/or aria-live) so users get immediate feedback.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
apps/web/src/routes/_app/dashboard.tsx (4)

30-31: Consider filtering instrument query by group.

useInstrumentInfoQuery is called without params, but later filtered in-memory by currentGroup.accessibleInstrumentIds (lines 40-44). If the API supports group-based filtering, passing params: { groupId: currentGroup?.id } would reduce payload size and improve performance.


40-62: Consider memoizing expensive data transformations.

The instrumentData, instrumentInfo, and recordCounter computations run on every render. If recordsQuery.data or instrumentInfoQuery.data contain many items, the filtering and counting operations (especially lines 56-62) could impact performance. Consider wrapping these in useMemo with appropriate dependencies.

🔎 Example using useMemo
+import React, { useMemo, useState } from 'react';

-  const instrumentData = currentGroup
-    ? instrumentInfoQuery.data?.filter((instrument) => {
-        return currentGroup.accessibleInstrumentIds.includes(instrument.id);
-      })
-    : instrumentInfoQuery.data;
+  const instrumentData = useMemo(
+    () =>
+      currentGroup
+        ? instrumentInfoQuery.data?.filter((instrument) => {
+            return currentGroup.accessibleInstrumentIds.includes(instrument.id);
+          })
+        : instrumentInfoQuery.data,
+    [currentGroup, instrumentInfoQuery.data]
+  );

-  const instrumentInfo = instrumentData?.map((instrument) => {
-    return {
-      id: instrument.id,
-      kind: instrument.kind,
-      title: instrument.details.title
-    };
-  });
+  const instrumentInfo = useMemo(
+    () =>
+      instrumentData?.map((instrument) => ({
+        id: instrument.id,
+        kind: instrument.kind,
+        title: instrument.details.title
+      })),
+    [instrumentData]
+  );

-  const recordIds = recordsQuery.data?.map((record) => record.instrumentId);
+  const recordIds = useMemo(() => recordsQuery.data?.map((record) => record.instrumentId), [recordsQuery.data]);

-  const recordCounter =
-    instrumentInfo?.map((title) => {
-      return {
-        count: recordIds?.filter((val) => val === title.id).length ?? 0,
-        instrumentTitle: title.title
-      };
-    }) ?? [];
+  const recordCounter = useMemo(
+    () =>
+      instrumentInfo?.map((title) => ({
+        count: recordIds?.filter((val) => val === title.id).length ?? 0,
+        instrumentTitle: title.title
+      })) ?? [],
+    [instrumentInfo, recordIds]
+  );

204-232: Add empty state for when no users exist.

The modal now handles loading and error states well (addressing the previous review). However, if userInfoQuery.data is an empty array, the modal displays only the header with no feedback. Consider adding an empty state message.

🔎 Example empty state
  <ul className="flex flex-col gap-5 overflow-auto">
    {userInfoQuery.isLoading && <Spinner />}
    {userInfoQuery.isError && (
      <p>
        {t({
          en: 'Error finding users',
          fr: "erreur lors de la recherche d'utilisateurs"
        })}
      </p>
    )}
+   {userInfoQuery.data?.length === 0 && (
+     <p>
+       {t({
+         en: 'No users found',
+         fr: 'Aucun utilisateur trouvé'
+       })}
+     </p>
+   )}
    <AnimatePresence mode="popLayout">
      {userInfoQuery.data?.map((user, i) => {

217-223: Cap animation delays for large lists.

The staggered animation delay 0.15 * i (lines 223, 306, 365) could result in very long delays for large lists. For example, 50 items would delay the last item by 7.5 seconds. Consider capping the delay (e.g., Math.min(0.15 * i, 1.5)) or removing the stagger for lists beyond a certain threshold.

🔎 Example with capped delay
  transition={{ bounce: 0.2, delay: Math.min(0.15 * i, 1.5), duration: 1.5, type: 'spring' }}

Also applies to: 300-306, 359-365

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c77f9be and 5982040.

📒 Files selected for processing (1)
  • apps/web/src/routes/_app/dashboard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/routes/_app/dashboard.tsx (3)
apps/web/src/hooks/useInstrumentInfoQuery.ts (1)
  • useInstrumentInfoQuery (15-29)
apps/web/src/hooks/useUsersQuery.ts (1)
  • useUsersQuery (17-19)
apps/web/src/hooks/useInstrumentRecords.ts (1)
  • useInstrumentRecords (12-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: lint-and-test
🔇 Additional comments (2)
apps/web/src/routes/_app/dashboard.tsx (2)

236-256: LGTM: Proper semantic button with navigation.

Using a button element for the clickable Subjects card is semantically correct and accessible. The navigation implementation is straightforward.


33-38: No action needed—the API handles undefined groupId gracefully.

When groupId is undefined, Prisma ignores it in the where clause (lines 245, 286 of InstrumentRecordsService), returning all records the user can access rather than throwing an error or matching NULL values. The unconditional enabled: true is safe.

Comment on lines 286 to 315
<ul className="flex flex-col gap-5 overflow-auto">
<AnimatePresence mode="popLayout">
<div className="flex justify-between gap-4 font-bold">
<p>
{t({
en: 'Title',
fr: 'Titre'
})}
</p>{' '}
<p>{t({ en: 'Kind', fr: 'Genre' })}</p>
</div>
<hr></hr>
{instrumentInfo?.map((instrument, i) => {
return (
<motion.li
layout
animate={{ opacity: 1, y: 0 }}
exit={{ opacity: 0 }}
initial={{ opacity: 0 }}
key={instrument.title}
transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
>
<div className="flex justify-between gap-4">
<p>{instrument.title}</p> <p>{instrument.kind}</p>
</div>
</motion.li>
);
})}
</AnimatePresence>
</ul>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add loading/error/empty states and use stable keys.

Similar to the Users modal, this should handle:

  • Loading state when instrumentInfoQuery.isLoading
  • Error state when instrumentInfoQuery.isError
  • Empty state when instrumentInfo is an empty array

Additionally, using instrument.title as the key (line 305) could cause issues if titles aren't unique. Use instrument.id instead for a stable, unique key.

🔎 Proposed fix
  <ul className="flex flex-col gap-5 overflow-auto">
+   {instrumentInfoQuery.isLoading && <Spinner />}
+   {instrumentInfoQuery.isError && (
+     <p>
+       {t({
+         en: 'Error loading instruments',
+         fr: 'Erreur de chargement des instruments'
+       })}
+     </p>
+   )}
+   {instrumentInfo?.length === 0 && (
+     <p>
+       {t({
+         en: 'No instruments available',
+         fr: 'Aucun instrument disponible'
+       })}
+     </p>
+   )}
    <AnimatePresence mode="popLayout">
      <div className="flex justify-between gap-4 font-bold">
        <p>
          {t({
            en: 'Title',
            fr: 'Titre'
          })}
        </p>{' '}
        <p>{t({ en: 'Kind', fr: 'Genre' })}</p>
      </div>
      <hr></hr>
      {instrumentInfo?.map((instrument, i) => {
        return (
          <motion.li
            layout
            animate={{ opacity: 1, y: 0 }}
            exit={{ opacity: 0 }}
            initial={{ opacity: 0 }}
-           key={instrument.title}
+           key={instrument.id}
            transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
          >
            <div className="flex justify-between gap-4">
              <p>{instrument.title}</p> <p>{instrument.kind}</p>
            </div>
          </motion.li>
        );
      })}
    </AnimatePresence>
  </ul>

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In apps/web/src/routes/_app/dashboard.tsx around lines 286 to 315, the list
rendering currently lacks loading, error and empty states and uses an unstable
key (instrument.title). Update the JSX to: check instrumentInfoQuery.isLoading
and render a compact loading indicator; check instrumentInfoQuery.isError and
render an error message or retry control; if instrumentInfo is an empty array
render an empty-state message; and when mapping instruments use a stable unique
key (instrument.id) instead of instrument.title; keep the AnimatePresence/motion
list structure and use the same layout/transition props when rendering each
instrument.

Comment on lines +345 to +374
<ul className="flex flex-col gap-5 overflow-auto">
<AnimatePresence mode="popLayout">
<div className="flex justify-between gap-4 font-bold">
<p>
{t({
en: 'Title',
fr: 'Titre'
})}
</p>{' '}
<p>{t({ en: 'Number', fr: 'Numero' })}</p>
</div>
<hr></hr>
{recordCounter?.map((instrument, i) => {
return (
<motion.li
layout
animate={{ opacity: 1, y: 0 }}
exit={{ opacity: 0 }}
initial={{ opacity: 0 }}
key={instrument.instrumentTitle}
transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
>
<div className="flex justify-between gap-4">
<p>{instrument.instrumentTitle}</p> <p>{instrument.count}</p>
</div>
</motion.li>
);
})}
</AnimatePresence>
</ul>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add loading/error/empty states and improve keys.

The Records modal should handle loading/error/empty states for recordsQuery. Additionally, using instrument.instrumentTitle as the key (line 364) may not be unique. Since recordCounter is derived from instrumentInfo, consider preserving id in the recordCounter objects and using it as the key.

🔎 Proposed fix

First, update the recordCounter computation to include id:

  const recordCounter =
    instrumentInfo?.map((title) => {
      return {
        count: recordIds?.filter((val) => val === title.id).length ?? 0,
+       id: title.id,
        instrumentTitle: title.title
      };
    }) ?? [];

Then update the modal:

  <ul className="flex flex-col gap-5 overflow-auto">
+   {recordsQuery.isLoading && <Spinner />}
+   {recordsQuery.isError && (
+     <p>
+       {t({
+         en: 'Error loading records',
+         fr: 'Erreur de chargement des enregistrements'
+       })}
+     </p>
+   )}
+   {recordCounter.length === 0 && !recordsQuery.isLoading && (
+     <p>
+       {t({
+         en: 'No records found',
+         fr: 'Aucun enregistrement trouvé'
+       })}
+     </p>
+   )}
    <AnimatePresence mode="popLayout">
      <div className="flex justify-between gap-4 font-bold">
        <p>
          {t({
            en: 'Title',
            fr: 'Titre'
          })}
        </p>{' '}
        <p>{t({ en: 'Number', fr: 'Numero' })}</p>
      </div>
      <hr></hr>
      {recordCounter?.map((instrument, i) => {
        return (
          <motion.li
            layout
            animate={{ opacity: 1, y: 0 }}
            exit={{ opacity: 0 }}
            initial={{ opacity: 0 }}
-           key={instrument.instrumentTitle}
+           key={instrument.id}
            transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
          >
            <div className="flex justify-between gap-4">
              <p>{instrument.instrumentTitle}</p> <p>{instrument.count}</p>
            </div>
          </motion.li>
        );
      })}
    </AnimatePresence>
  </ul>

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
apps/web/src/routes/_app/dashboard.tsx (2)

286-315: Missing loading/error states and unstable key.

This modal still lacks:

  1. Loading state when instrumentInfoQuery.isLoading
  2. Error state when instrumentInfoQuery.isError
  3. Empty state when instrumentInfo is empty

Also, instrument.title (line 305) may not be unique. Use instrument.id as the key.

Proposed fix
  <ul className="flex flex-col gap-5 overflow-auto">
+   {instrumentInfoQuery.isLoading && <Spinner />}
+   {instrumentInfoQuery.isError && (
+     <p>{t({ en: 'Error loading instruments', fr: 'Erreur de chargement des instruments' })}</p>
+   )}
    <AnimatePresence mode="popLayout">
      <div className="flex justify-between gap-4 font-bold">
        ...
      </div>
      <hr></hr>
      {instrumentInfo?.map((instrument, i) => {
        return (
          <motion.li
            ...
-           key={instrument.title}
+           key={instrument.id}
            ...
          >

345-374: Missing loading/error states and unstable key.

This modal also needs:

  1. Loading state when recordsQuery.isLoading
  2. Error state when recordsQuery.isError
  3. Empty state when recordCounter is empty

Use instrument.id as the key (line 364) after adding it to recordCounter.

Proposed fix
  <ul className="flex flex-col gap-5 overflow-auto">
+   {recordsQuery.isLoading && <Spinner />}
+   {recordsQuery.isError && (
+     <p>{t({ en: 'Error loading records', fr: 'Erreur de chargement des enregistrements' })}</p>
+   )}
    <AnimatePresence mode="popLayout">
      ...
      {recordCounter?.map((instrument, i) => {
        return (
          <motion.li
            ...
-           key={instrument.instrumentTitle}
+           key={instrument.id}
            ...
          >
🧹 Nitpick comments (1)
apps/web/src/routes/_app/dashboard.tsx (1)

56-62: Include id in recordCounter for stable list keys.

The recordCounter objects lack an id field. Without it, the Records modal must use instrumentTitle as a key, which may not be unique. Include the id to enable stable React keys downstream.

Proposed fix
  const recordCounter =
    instrumentInfo?.map((title) => {
      return {
        count: recordIds?.filter((val) => val === title.id).length ?? 0,
+       id: title.id,
        instrumentTitle: title.title
      };
    }) ?? [];
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5982040 and 1c69af8.

📒 Files selected for processing (1)
  • apps/web/src/routes/_app/dashboard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/routes/_app/dashboard.tsx (1)
vendor/zod@3.x/src/v3.d.ts (1)
  • record (2499-2499)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: lint-and-test
🔇 Additional comments (1)
apps/web/src/routes/_app/dashboard.tsx (1)

236-256: LGTM!

Using a semantic <button> element with navigation on click is appropriate. The void keyword properly handles the returned promise.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
apps/web/src/routes/_app/dashboard.tsx (2)

287-316: Add loading/error/empty states.

This modal still lacks loading, error, and empty state handling for instrumentInfoQuery. Without these, users see no feedback during data fetch or on failure.

🔎 Recommended fix
  <ul className="flex flex-col gap-5 overflow-auto">
+   {instrumentInfoQuery.isLoading && <Spinner />}
+   {instrumentInfoQuery.isError && (
+     <p>
+       {t({
+         en: 'Error loading instruments',
+         fr: 'Erreur de chargement des instruments'
+       })}
+     </p>
+   )}
+   {instrumentInfo?.length === 0 && !instrumentInfoQuery.isLoading && (
+     <p>
+       {t({
+         en: 'No instruments available',
+         fr: 'Aucun instrument disponible'
+       })}
+     </p>
+   )}
    <AnimatePresence mode="popLayout">
      <div className="flex justify-between gap-4 font-bold">

346-375: Add loading/error/empty states and use stable key.

This modal lacks loading/error/empty state handling and uses instrument.instrumentTitle as the key (line 365), which may not be unique. Use instrument.id instead since it's already included in recordCounter (line 59).

🔎 Recommended fix
  <ul className="flex flex-col gap-5 overflow-auto">
+   {recordsQuery.isLoading && <Spinner />}
+   {recordsQuery.isError && (
+     <p>
+       {t({
+         en: 'Error loading records',
+         fr: 'Erreur de chargement des enregistrements'
+       })}
+     </p>
+   )}
+   {recordCounter.length === 0 && !recordsQuery.isLoading && (
+     <p>
+       {t({
+         en: 'No records found',
+         fr: 'Aucun enregistrement trouvé'
+       })}
+     </p>
+   )}
    <AnimatePresence mode="popLayout">
      <div className="flex justify-between gap-4 font-bold">
        <p>
          {t({
            en: 'Title',
            fr: 'Titre'
          })}
        </p>{' '}
        <p>{t({ en: 'Number', fr: 'Numero' })}</p>
      </div>
      <hr></hr>
      {recordCounter?.map((instrument, i) => {
        return (
          <motion.li
            layout
            animate={{ opacity: 1, y: 0 }}
            exit={{ opacity: 0 }}
            initial={{ opacity: 0 }}
-           key={instrument.instrumentTitle}
+           key={instrument.id}
            transition={{ bounce: 0.2, delay: 0.15 * i, duration: 1.5, type: 'spring' }}
          >
            <div className="flex justify-between gap-4">
              <p>{instrument.instrumentTitle}</p> <p>{instrument.count}</p>
            </div>
          </motion.li>
        );
      })}
    </AnimatePresence>
  </ul>
🧹 Nitpick comments (1)
apps/web/src/routes/_app/dashboard.tsx (1)

30-38: Consider enabling queries conditionally.

All queries run immediately on mount, even though the modals start closed. For better performance, consider enabling each query only when its modal opens.

🔎 Example optimization
- const instrumentInfoQuery = useInstrumentInfoQuery();
+ const instrumentInfoQuery = useInstrumentInfoQuery({ 
+   enabled: isInstrumentModalOpen 
+ });
- const userInfoQuery = useUsersQuery();
+ const userInfoQuery = useUsersQuery({ 
+   enabled: isUserModalOpen 
+ });
  const recordsQuery = useInstrumentRecords({
-   enabled: true,
+   enabled: isRecordModalOpen,
    params: {
      groupId: currentGroup?.id
    }
  });

Note: Verify that the hooks support an enabled option.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c69af8 and 82b73af.

📒 Files selected for processing (1)
  • apps/web/src/routes/_app/dashboard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/routes/_app/dashboard.tsx (3)
apps/web/src/hooks/useInstrumentInfoQuery.ts (1)
  • useInstrumentInfoQuery (15-29)
apps/web/src/hooks/useUsersQuery.ts (1)
  • useUsersQuery (17-19)
apps/web/src/hooks/useInstrumentRecords.ts (1)
  • useInstrumentRecords (12-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: lint-and-test
🔇 Additional comments (5)
apps/web/src/routes/_app/dashboard.tsx (5)

1-16: LGTM!

All necessary imports are present for the modal interactions, animations, and data fetching.


27-29: LGTM!

Modal state management is straightforward and correct.


40-63: LGTM!

Data processing logic is correct with appropriate optional chaining and includes stable IDs for rendering.


205-233: LGTM! Past issues addressed.

Loading and error states are now properly handled, and optional chaining prevents runtime crashes.


237-257: LGTM!

Navigation implementation is correct. The void keyword appropriately indicates the intentionally ignored promise.

Copy link
Collaborator

@joshunrau joshunrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to m,e

@joshunrau joshunrau merged commit 4b967b4 into DouglasNeuroInformatics:main Jan 2, 2026
2 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants